Switch to using feature for RequestServices #369
Conversation
|
Why? Bug #? |
|
@Tratcher Performance. Making this lazy. The only problem is the fact that IServiceProviders feature is pretty hidden. |
| private IServiceScope _scope; | ||
| private bool _requestServicesSet; | ||
|
|
||
| public RequestServicesFeature(IServiceProvider applicationServices, IServiceScopeFactory scopeFactory) |
There was a problem hiding this comment.
Do we even need to resolve the factory this early?
There was a problem hiding this comment.
Resolve it from AppServices in the getter instead?
|
Added a test to verify we no-op if there's an existing ISPsFeature. Removed IServiceScope from ctors and just GetRequiredService when needed |
|
|
||
| public void Dispose() | ||
| { | ||
| if (_scope != null) |
There was a problem hiding this comment.
I was gonna say this is so old school:
_scope?.Dispose();
There was a problem hiding this comment.
Sure, can just nuke the if then
| { | ||
| if (!_requestServicesSet) | ||
| { | ||
| _scope = ApplicationServices.GetRequiredService<IServiceScopeFactory>().CreateScope(); |
There was a problem hiding this comment.
Does this need to be defensive since ApplicationServices is settable?
There was a problem hiding this comment.
Yeah looks like it. Do we want to throw an exception if you try to set it to null, or just treat that as no-op and basically do nothing and leave RequestServices null if you set ApplicationServices to null?
There was a problem hiding this comment.
This behavior is totally undefined. To repro this you would have to set ApplicationServices to null then before ever touching RequestServices (I just know somebody is going to do something stupid and report a bug 😄). This works today because nothing is lazy. We could store the original application services and use that as a fallback but that might be perplexing. Throwing might be the most reasonable thing. Null isn't bad either so lets flip a coin. If you did this as a customer an exception might be the best since you'd want to know why instead of getting a null ref. On the other hand if you get RequestServices and then set ApplicationServices to null, should that throw?
There was a problem hiding this comment.
Is there any real scenario where we want to allow them to set ApplicationServices to null? If not, I can just un auto the property and throw for null sets... Won't we fall over all over the place if ApplicationServices is null? I don't think we are checking anywhere in our code today
There was a problem hiding this comment.
I don't know why anybody would set it to null unless they were trying to clear it out. Maybe if the old value was null?
There was a problem hiding this comment.
Ok well this is a subtle breaking change then, setting to null will throw an exception now... updated and added a test
| { | ||
| if (value == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(ApplicationServices)); |
There was a problem hiding this comment.
This should be value not ApplicationServices afaik
|
Small change then |
once CI is green
No description provided.